Converted Talking Book Tool to React (BL-16410)#8075
Conversation
A design/hand-off document (Steps 0-5) describing how to convert the last non-React toolbox tool to React: move the AudioRecording engine to the page iframe, turn the button-state machine and controls into a React component in the toolbox, and stop audioRecording.ts from being duplicated into the page bundle. Step 6 (retiring the legacy non-React toolbox framework) is documented as a deferred follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…BloomDesktop into talkingBookToolReact
|
| Filename | Overview |
|---|---|
| src/BloomBrowserUI/bookEdit/toolbox/talkingBook/TalkingBookToolControls.tsx | New React component implementing the Talking Book Tool UI; connects to the engine via registerStateListener; MUI Menu open/anchor sequencing is correct. |
| src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts | Major refactor removing delegated jQuery handlers in favour of React state callbacks; previously flagged issues (shouldShowDeviceMenu reset, showAdjustTimingsDialog args) are now addressed. |
| src/BloomBrowserUI/bookEdit/toolbox/talkingBook/talkingBookTool.tsx | New ToolboxToolReactAdaptor subclass wiring TalkingBookToolControls into the toolbox; lifecycle methods (showTool, newPageReady, hideTool, detachFromPage) look complete. |
| src/BloomBrowserUI/bookEdit/toolbox/talkingBook/TalkingBookUiState.ts | Clean state interface; DisabledUnlessHover enum value removed intentionally as part of React conversion. |
| src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts | Legacy pug-tool branch removed now that TalkingBookTool is a full React tool; code simplified cleanly. |
| src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts | Tests updated to use uiState-based assertions instead of DOM class checks; SetupTalkingBookUIElements removed since elements are now lazily created by the engine. |
Reviews (14): Last reviewed commit: "Moved some files around" | Re-trigger Greptile
12cc862 to
05064c7
Compare
24bcd41 to
19b45f5
Compare
19b45f5 to
5191780
Compare
A design/hand-off document (Steps 0-5) describing how to convert the last non-React toolbox tool to React: move the AudioRecording engine to the page iframe, turn the button-state machine and controls into a React component in the toolbox, and stop audioRecording.ts from being duplicated into the page bundle. Step 6 (retiring the legacy non-React toolbox framework) is documented as a deferred follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5191780 to
552d600
Compare
andrew-polk
left a comment
There was a problem hiding this comment.
@andrew-polk partially reviewed 27 files and all commit messages, made 3 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on josiahwall).
src/BloomBrowserUI/bookEdit/js/audioUtils.ts line 1 at r3 (raw file):
// taken out of audioRecording.ts to avoid the need for other
I'm thinking all these new little utility files would fit better in toolbox/talkingBook rather than js. But I could be missing something.
What do you think?
src/BloomBrowserUI/bookEdit/toolbox/talkingBook/showTalkingBookTool.ts line 13 at r4 (raw file):
?.getTheOneToolbox() .activateToolFromId(kTalkingBookToolId); }
As far as I can tell, this file is not needed.
andrew-polk
left a comment
There was a problem hiding this comment.
@andrew-polk reviewed 2 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on josiahwall).
src/BloomBrowserUI/bookEdit/toolbox/talkingBook/showTalkingBookTool.ts line 13 at r4 (raw file):
Previously, andrew-polk wrote…
As far as I can tell, this file is not needed.
Hm. Not sure what I was seeing. Ignore.
andrew-polk
left a comment
There was a problem hiding this comment.
@andrew-polk partially reviewed 12 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on josiahwall).
src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts line 114 at r6 (raw file):
const kEndTimeAttributeName: string = "data-audioRecordingEndTimes"; const kPlaybackOrderContainerClass: string = "bloom-playbackOrderControlsContainer";
What was the reason for moving this back in here?
There just wasn't a good place for the file?
In the age of AI, it is unlikely that we would update one without finding the other, but the shared constant was a protection against that. But one can definitely make the argument it isn't worth a whole file.
Not necessarily asking for a change, just curious why you put it back.
josiahwall
left a comment
There was a problem hiding this comment.
@josiahwall made 2 comments and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on andrew-polk).
src/BloomBrowserUI/bookEdit/toolbox/talkingBook/showTalkingBookTool.ts line 13 at r4 (raw file):
Previously, andrew-polk wrote…
Hm. Not sure what I was seeing. Ignore.
Ignored.
src/BloomBrowserUI/bookEdit/js/audioUtils.ts line 1 at r3 (raw file):
Previously, andrew-polk wrote…
I'm thinking all these new little utility files would fit better in
toolbox/talkingBookrather thanjs. But I could be missing something.What do you think?
I moved all the files except for the xhtmlIdUtils.ts, because that was the only file that did not seem to be audio recording/talking book specific. The other ones made more sense to be in the talkingBook directory. Please let me know if I should move the other one too.
andrew-polk
left a comment
There was a problem hiding this comment.
@andrew-polk resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on josiahwall).
Devin review
This change is